I've marked this ready for review, @jakepetroules, @finagolfin, and @compnerd.

See also the discussion at swiftlang/swift-build#495 (comment)

I agree with getting rid of this. As much as the idea is to automatically set things up and make things automatically work, I think it's better to have the user configure this themselves. For example, this env variable is automatically set on the github CI, so even if the user explicitly configures the sdkRootPath to some other NDK, that will be overridden by this environment variable.

Better to not have such magic and make the setup more explicit, as it would be after this pull.

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This is going to break all of our builds

Then how about gating it within an #if os(Windows) check? This issue a blocker for Android builds on macOS and Linux, so we need some solution.

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This change isn't removing the ability to do that, right? It's just removing the default fallback. From what I understand, explicitly passing -sysroot is still gonna work, and that's what we want build systems to do anyways. Am I understanding incorrectly?

@compnerd Any thoughts on either @jakepetroules's questions or my suggestion? I'd really like to reach some solution so we can un-block the Android SDK work.

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This change isn't removing the ability to do that, right? It's just removing the default fallback. From what I understand, explicitly passing -sysroot is still gonna work, and that's what we want build systems to do anyways. Am I understanding incorrectly?

Ah, right; this still will break our builds as we do rely on the default. Can we wire up the default through swift-build before we remove this?

As far as I know, swift-build is already doing the right thing here, e.g. it will always pass a value for -sdk when compiling for Android.

@compnerd or @weliveindetail, could you apply this change to your CI and let us know if it causes any error and what the full output is? Because that may turn up an underlying bug that is better fixed rather than papering over it with this Android default.

This hasn't made it into the swift-6.2 brach, has it? I'm still seeing the build failures when I have ANDROID_NDK_ROOT set, which is especially problematic because the developer needs to set ANDROID_NDK_HOME to run the SDK setup script initially, but needs to un-set ANDROID_NDK_ROOT. These are both set to the same value in GitHub actions.

@finagolfin Can you add this to the project board?

Can we get this updated and landed? @compnerd do you still have concerns?

I think that we should wait until @finagolfin's frontend/driver changes are done and then re-test before merging this. The --sysroot flag is needed to allow us to pull content from the NDK without copying it into the Swift SDK, so I would prefer that we do not merge this change.

My frontend change in swiftlang/swift#79621 only affects -sdk usage, does not affect this -sysroot usage at all.

It is unclear why this env variable detection and -sysroot flag insertion is so important to your builds, as you could just detect it manually in your build scripts and add it in your config yourself. This is really just a convenience method for swift-driver to do that for you, but the problem is that it breaks other build environments that have multiple NDKs and don't use the env variable but manually pass in the NDK path.

Is there some ordering issue or something, on the TBC CI builds with this pull? If you guys would simply try this patch and replace it with manually passing in this flag to your build config, we could determine if this was really necessary to you or not, from the resulting error output, if any.

@marcprux, mind rebasing?

@swift-ci test

@swift-ci test windows

@Steelskin, can you apply this pull to your Windows CI, along with passing in this -sysroot ANDROID_NDK_ROOT flag manually through wherever your Android build config is, similar to how the Powershell script build.ps1 is already doing this, and report any build errors that you see?

The problem with this default is that it forces everyone to use the NDK at ANDROID_NDK_ROOT or have to unset that variable each time, when many have alternate NDKs they want to use through swift sdk configure instead, swiftlang/swift-package-manager#8856.

@Steelskin, can you apply this pull to your Windows CI, along with passing in this -sysroot ANDROID_NDK_ROOT flag manually through wherever your Android build config is, similar to how the Powershell script build.ps1 is already doing this, and report any build errors that you see?

The problem with this default is that it forces everyone to use the NDK at ANDROID_NDK_ROOT or have to unset that variable each time, when many have alternate NDKs they want to use through swift sdk configure instead, swiftlang/swift-package-manager#8856.

I am in the process of changing our build to more closely match what is being done in buid.ps1, but this is still a work in progress. In the meantime, it is going to be a bit challenging to get this to run so bear with me.
We run our smoke android test with the following:

          swift build `
            --package-path ${{ github.workspace }}/SourceCache/cassowary `
            --triple ${{ matrix.arch }}-unknown-linux-android${{ inputs.ANDROID_API_LEVEL }} `
            --sdk "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -sdk -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}" `
            -Xswiftc -sysroot -Xswiftc "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -I -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}\usr\include" `
            -Xswiftc -Xclang-linker -Xswiftc -resource-dir -Xswiftc -Xclang-linker -Xswiftc ${{ steps.android-swift-env.outputs.clang-resource-dir }}

Does that sound about correct to you or would you like me to edit some of the flags and/or the environment here?

Are you sure you need both --sdk sdk-path and -Xswiftc -sdk -Xswiftc sdk-path? The latter seems redundant, but perhaps there is a bug I'm unaware of.

Other than that, I'd like you to run these flags you've pasted but using a swift-driver with this pull of Marc's applied: see if removing this ANDROID_NDK_ROOT detection causes any problem on your CI when you're passing it in with -Xswiftc -sysroot -Xswiftc ndk-path instead.

Are you sure you need both --sdk sdk-path and -Xswiftc -sdk -Xswiftc sdk-path? The latter seems redundant, but perhaps there is a bug I'm unaware of.

Not sure, this is how the build is configured. I can toy with what arguments we actually need at a later time.

Other than that, I'd like you to run these flags you've pasted but using a swift-driver with this pull of Marc's applied: see if removing this ANDROID_NDK_ROOT detection causes any problem on your CI when you're passing it in with -Xswiftc -sysroot -Xswiftc ndk-path instead.

Alright, got it. All I did was change the swift-driver repo to point to swift-everywhere:main. The build is ongoing, I'll let you know how it goes.

@finagolfin Our CI succeeded with the current swift-everywhere:main branch.

@compnerd, if you would approve or let us know if you need to test anything else, I'd like to get this in.

Pinging @compnerd again, would like to get this into 6.2 next, as I have had to work around this override on my Android CI and so have others. If you have no further concerns, please approve and we'll get this in.

I still have concerns around this - the user needs to pass in -sysroot manually (which is what @Steelskin pointed out). As long as we that I can restore this change if the user needs to specify the path, I am okay with merging. The point is that the user shouldn't have to provide the paths. ANDROID_NDK_PATH is a variable that the NDK sets. Using that to locate the NDK unless the user overrides it explicitly seems reasonable.

Using that to locate the NDK unless the user overrides it explicitly seems reasonable.

The problem is that in general the ANDROID_NDK_ROOT variable is not set, so most of the time this adds nothing for the user. However, it is set in github CI and some other environments, but this pull passed the new -sysroot flag that almost nobody is using yet, plus --sysroot for the linker, so on such CI it depends if the user wants the default NDK using the new -sysroot flag. Since SwiftPM doesn't really support using such external C/C++ SDKs yet with the bundle feature we're pushing now, swiftlang/swift-package-manager#8958, almost nobody is using such a setup.

I understand that you guys at TBC got it to work, but because most people won't, I think it's best if you pass in the -sysroot manually for now. As we get more experience with this flag in the coming year, we can revisit this. What do you think?

The problem is that in general the ANDROID_NDK_ROOT variable is not set, so most of the time this adds nothing for the user.

This is especially true since ANDROID_NDK_ROOT is not the currently recommended environment variable to point to the NDK installation. It should be ANDROID_NDK_HOME (see https://github.com/android/ndk-samples/wiki and https://developer.android.com/ndk/guides/graphics/getting-started#creating).

This is the default variable that will be set by environments like Homebrew, while others (like Github runners) set both for good measure.

I would be okay with changing the behaviour to refer to ANDROID_NDK_HOME rather than ANDROID_NDK_ROOT. I wonder how that got mixed up.

BTW, I don't think that we should be passing in the other flags - that sounds like something we should clean up. The idea is that with ANDROID_NDK_ROOT (well, ANDROID_NDK_HOME) set, all the appropriate flags should be computed in the driver.

I don't think that we should be passing in the other flags - that sounds like something we should clean up.

Which other flags are you referring to here?

The idea is that with ANDROID_NDK_ROOT (well, ANDROID_NDK_HOME) set, all the appropriate flags should be computed in the driver.

If by that you mean -sysroot, which this pull removes, that flag does not work well yet, so setting it as the default is more likely to break builds.

@compnerd, now that this is not needed for your TBC CI, can we go ahead and get it in? We all agree that we should simplify these Android flags much more, but we are still a fair ways away from that:

1. Even with this env variable checked, your Windows cross-compilation for Android currently requires a ton of flags:

swift build `
            --package-path ${{ github.workspace }}/SourceCache/cassowary `
            --triple ${{ matrix.arch }}-unknown-linux-android${{ inputs.ANDROID_API_LEVEL }} `
            --sdk "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -sdk -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}" `
            -Xswiftc -sysroot -Xswiftc "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -I -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}\usr\include" `
            -Xswiftc -Xclang-linker -Xswiftc -resource-dir -Xswiftc -Xclang-linker -Xswiftc ${{ steps.android-swift-env.outputs.clang-resource-dir }}

2. This env variable checking, that Marc reverts in this pull, currently breaks cross-compilation to Android on all non-TBC CI.

The good news is that SDK bundles already abstract away most of these flags for users, though I know those don't work on Windows or for user-installed platform C/C++ SDKs, like the Android NDK, yet.

Let's just fix this breakage for now, and we can whittle away those extra Android flags over time.

Reading through this in more detail to try to ensure I understand correctly...

There are two flags relevant for the Swift compiler to build for Android:

• -sdk: path to the Swift SDK (Swift-specific bits)
• --sysroot: path to the Android NDK sysroot

Are those flags working in the driver today and is SwiftPM passing them both?

For example, this env variable is automatically set on the github CI, so even if the user explicitly configures the sdkRootPath to some other NDK, that will be overridden by this environment variable.

I am confused about this part. Why would that be the case? sdkRootPath maps to --sysroot, which when passed will override any environment that is set, right?

There are two flags relevant for the Swift compiler to build for Android:
-sdk: path to the Swift SDK (Swift-specific bits)
--sysroot: path to the Android NDK sysroot
Are those flags working in the driver today and is SwiftPM passing them both?

Close, historically, the two flags used to cross-compile are -sdk, for the platform C/C++ sysroot, and -resource-dir, for the Swift runtime modules and libraries. Recently, the -sysroot flag was added and switching to -sdk/-sysroot over time was proposed, but that largely has not been implemented yet.

This pull reverts the env variable checking because it inserts that new -sysroot flag regardless of which pair of cross-compilation flags above are being used, breaking most current Android cross-compilation with the -sdk/-resource-dir flags.

You'll notice that the long TBC CI command above is an exception that does not pass the -resource-dir flag to the Swift compiler, though it does pass in the different -Xclang-linker -resource-dir -Xclang-linker ${{ steps.android-swift-env.outputs.clang-resource-dir }} flag to clang. They apparently get away with this because they don't use Swift SDK bundles on Windows, where bundles don't work yet, and by manually passing in all those extra flags.

I am confused about this part. Why would that be the case? sdkRootPath maps to --sysroot, which when passed will override any environment that is set, right?

sdkRootPath in a Swift bundle is used to set the -sdk flag when compiling. It is also separately passed with --sysroot to the Swift clangImporter, but the bigger issue is that you are now passing in two different platform C/C++ sysroots, one with -sdk and another with -sysroot in the code this pull changes, which causes conflicts.

It is not worth getting into all the details, but the basic point is the meaning of -sdk changes, depending on whether it is used with the current standard of -resource-dir or the proposed new -sysroot. In the former, -sdk is the platform C/C++ SDK, whereas in the latter, it is the path to the Swift runtime libraries. You cannot simply pass in -sysroot no matter what, which is why this revert is needed.

Thanks for the explanation, this is making things a little clearer. Can you also clarify whether you're building via SwiftPM, or building with a different build system?

If sdkRootPath is supposed to correspond to the Swift SDK bits and not the platform sysroot, is SwiftPM doing the right thing today? It looks like -sdk is being passed for Swift, but this one for the C compiler needs correction.

if let sdkDir = swiftSDK.pathsConfiguration.sdkRootPath {
    let sysrootFlags = [triple.isDarwin() ? "-isysroot" : "--sysroot", sdkDir.pathString]
    self.extraFlags.cCompilerFlags.insert(contentsOf: sysrootFlags, at: 0)
}

but that largely has not been implemented yet.

What's still missing?

This pull reverts the env variable checking because it inserts that new -sysroot flag regardless of which pair of cross-compilation flags above are being used, breaking most current Android cross-compilation with the -sdk/-resource-dir flags.

Could this have some smarter detection? e.g. don't add the default --sysroot flag if -sdk is set and points to something that looks like an NDK sysroot?

Can you also clarify whether you're building via SwiftPM, or building with a different build system?

The build system is irrelevant, as these are Swift compiler flags that any build system has to use. I merely focused on SwiftPM because it is what most use, but these Swift compiler flags used for cross-compilation are the same regardless of build system, whether CMake or Bazel.

If sdkRootPath is supposed to correspond to the Swift SDK bits and not the platform sysroot, is SwiftPM doing the right thing today?

OK, I can see you are confused, so let me get into the slightly longer expanation. Until recently, there were supposed to be two options to cross-compile using the -sdk flag. You could specify "-sdk, for the platform C/C++ sysroot" alone, contrary to what you just wrote, and "-resource-dir, for the Swift runtime modules and libraries." If you happened to have a non-Darwin C/C++ sysroot that also contained the Swift runtime in <sdk>/usr/lib/swift/, you could specify -sdk alone, but only with the legacy C++ Driver: that was never brought over to this new swift-driver (I'm working on a pull to bring that capability over to the swift-frontend, which this swift-driver invokes to find such flags, swiftlang/swift#79621).

Well, when the -sdk/-sysroot switch was proposed in the cross-compilation document I linked above, my guess is that its authors thought they could reuse that capability to look in -sdk for the Swift runtime libraries instead, and split off the platform C/C++ sysroot into the new -sysroot flag. However, they were likely unaware that this new swift-driver does not look in -sdk for the Swift runtime libraries, and their effort to switch to -sdk/-sysroot appears to have collapsed.

The SwiftPM flag you pasted is correct under the -sdk/-resource-dir cross-compilation model almost everybody is now using, but you are right that it may cause problems if we ever made the switch to the -sdk/-sysroot model that almost nobody is now using and is mostly unimplemented.

The only reason the TBC CI is able to use -sdk/-sysroot above is because either Swift cross-compilation flag pair is then used to automatically set a bunch of -I/-L/--sysroot flags internal to the Swift compiler, so they replace that with a bunch of manually passed-in flags like those shown above. That is brittle and tends to break though, better to choose a pair of front-facing cross-compilation flags and keep it simple.

What's still missing?

The main one is actually having this swift-driver look in -sdk for the Swift runtime libraries! After a year of my pointing out that that basic capability doesn't work to those who wanted to switch to -sdk/-sysroot and not getting a response until recently, I finally stepped up to implement it myself. Another is that -sysroot still has bugs, since almost nobody is using it.

Could this have some smarter detection? e.g. don't add the default --sysroot flag if -sdk is set and points to something that looks like an NDK sysroot?

Given all the -sdk/-sysroot problems detailed above, best to just take it out for now, as Marc does in this pull. Another issue is that there's no guarantee ANDROID_NDK_ROOT is the NDK that users want, so we'd need to have a story in place of how to override it.

A final aspect that I don't like is that we have an explicit flag for this in SwiftPM, better to use that than these magic environment variables. You think you're making life easier for the user, but you're really just introducing hidden variables that can break things in hard to debug ways.

Let me finally note that I'm not against the -sdk/-sysroot switch, but I do have doubts that it can be made to work and most importantly, we shouldn't be pushing in this -sysroot flag for Android alone until the underlying -sdk/-sysroot support is fully implemented.